Skip to content

fix(cli): only route auxiliary tasks to kilo-auto/small with kilo credentials - #13192

Merged
WebReflection merged 11 commits into
Kilo-Org:mainfrom
maphew:fix/aux-task-small-model-fallback
Sep 11, 2026
Merged

WebReflection merged 11 commits into
Kilo-Org:mainfrom
maphew:fix/aux-task-small-model-fallback

Conversation

@maphew

@maphew maphew commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Fixes #13151

TL;DR

getSmallModel always fell back to the cloud kilo-auto/small model because the kilo provider is autoloaded with an anonymous key. Session title generation, commit messages, and branch names were silently routed to an unreachable cloud model for users without kilo access — offline/local-only setups saw titles never update (7.4.22 regression, see #13151; likely contributor to #12962 commit-message timeouts).

Change

  • packages/opencode/src/provider/provider.ts: gate the kilo-auto/small fallback on actual kilo credentials
  • packages/opencode/src/kilocode/provider/provider.ts: new hasKiloCredentials() helper mirroring the kilo loader's hasKey check (KILO_API_KEY env, stored auth entry, or config provider.kilo.options.apiKey)
  • packages/opencode/test/provider/provider.test.ts: split the old "always falls back" test into a no-credentials case (expects undefined → callers use the session's own model) and a configured case (still returns kilo-auto/small)
  • changeset added

Verification

  • bun run typecheck (packages/opencode): pass
  • bun test test/provider/provider.test.ts --test-name-pattern "getSmallModel": 10 pass / 0 fail
  • Full provider.test.ts: 97 pass / 3 fail — the 3 failures (defaultModel respects config, defaultModel typed error, opencode loader paid models) are pre-existing on the base commit in this sandbox (network/model-cache related), unchanged by this PR

Note: pre-push hook typecheck fails only on @kilocode/kilo-jetbrains (no Java in sandbox); opencode package typecheck is green.

maphew and others added 4 commits August 18, 2026 08:01
…entials

getSmallModel unconditionally fell back to the cloud kilo-auto/small model
because the kilo provider is always autoloaded with an anonymous key. For
users without kilo access (offline or local-only setups), session title,
commit message, and branch name generation were routed to an unreachable
model and failed silently.

Gate the fallback on actual kilo credentials (KILO_API_KEY env, auth entry,
or config apiKey) so those auxiliary tasks use the session's own model.
…model-fallback

Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
@maphew
maphew marked this pull request as ready for review August 22, 2026 03:00

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1dc515ff0d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/opencode/test/provider/provider.test.ts Outdated
@kilo-code-bot

kilo-code-bot Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 files)
  • packages/opencode/test/kilocode/session-prompt-compaction-safety.test.ts
Previous Review Summaries (2 snapshots, latest commit 466ae2a)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 466ae2a)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • packages/opencode/test/kilocode/provider/provider.test.ts
  • packages/opencode/test/provider/provider.test.ts

Previous review (commit 1dc515f)

Status: 1 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
packages/opencode/test/provider/provider.test.ts 781 No-credentials test never clears KILO_API_KEY, so Env.all() can still treat kilo as authenticated and flake
Files Reviewed (4 files)
  • .changeset/small-model-fallback-requires-kilo-credentials.md - 0 issues
  • packages/opencode/src/kilocode/provider/provider.ts - 0 issues
  • packages/opencode/src/provider/provider.ts - 0 issues
  • packages/opencode/test/provider/provider.test.ts - 1 issue

Fix these issues in Kilo Cloud


Reviewed by grok-4.6 · Input: 87.9K · Output: 7.3K · Cached: 470.5K

Review guidance: REVIEW.md from base branch main

maphew commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both review findings in 466ae2a:

  • Codex P1 (fork isolation): the two getSmallModel regression cases moved to test/kilocode/provider/provider.test.ts; the shared test/provider/provider.test.ts no longer carries any kilo-only cases (the obsolete unconditional-fallback test was removed).
  • kilo review WARNING (env flake): the no-credentials test now clears KILO_API_KEY (via Env.use.remove) in addition to KILO_AUTH_CONTENT/KILO_CONFIG_CONTENT, so a CI-host KILO_API_KEY can no longer make it flake.

Verified locally: both relocated tests pass, packages/opencode typecheck and the kilocode-change annotation check pass. The branch is up to date with main.

maphew commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Re-triggering CI after a flaky unit (windows, 3/6) timeout in the unrelated issue-8656 stall test (passed on the same shard at the previous commit; head commit only moves test files).

@maphew maphew closed this Aug 22, 2026
@maphew maphew reopened this Aug 22, 2026
maphew and others added 3 commits August 27, 2026 06:41
…model-fallback

Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
Once getSmallModel no longer falls back to the kilo cloud model without
credentials, auxiliary session-title generation resolves to the session's
own model. In the mock-server compaction safety tests the session model
is the test model, so the forked title call hits the mock server and
shifts the exact llm.calls assertions by one.

Create those sessions with explicit titles so ensureTitle early-returns
and the call counts keep asserting the compaction behavior only.

Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>

maphew commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

Merge-readiness check complete:

  • Codex finding addressed: regression tests moved to test/kilocode/provider/provider.test.ts (466ae2a); shared test file no longer carries kilo-specific cases. The no-credentials test also clears KILO_API_KEY to fix the flagged flake.
  • Branch synced with main (0 behind), no merge conflicts.
  • CI green (all checks pass).
  • Typecheck + provider tests verified locally.

Ready for human review.

maphew added 2 commits August 31, 2026 03:42
Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
…x-task-small-model-fallback

Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>

maphew commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Merge-readiness sweep complete: branch synced with current main (ffc0655, 0 behind), no merge conflicts, CI green on the latest head. Ready for human review.

…model-fallback

Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
@kilo-code-bot

kilo-code-bot Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

To use Kilo from GitHub you first need to link your GitHub account to Kilo. Link your Kilo account to continue. After linking, mention me again in this issue or pull request.

@WebReflection

Copy link
Copy Markdown
Contributor

@kilocode-bot can you please check again? The opencode change without surrounding comments does not look right but your last check was also weeks ago, thanks.

@kilo-code-bot

kilo-code-bot Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

To use Kilo from GitHub you first need to link your GitHub account to Kilo. Link your Kilo account to continue. After linking, mention me again in this issue or pull request.

@WebReflection

Copy link
Copy Markdown
Contributor

@kilocode-bot can you please check again? The opencode change without surrounding comments does not look right but your last check was also weeks ago, thanks.

@kilo-code-bot

kilo-code-bot Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

To use Kilo from GitHub you first need to link your GitHub account to Kilo. Link your Kilo account to continue. After linking, mention me again in this issue or pull request.

@WebReflection

Copy link
Copy Markdown
Contributor

@kilocode-bot can you please check again? The opencode change without surrounding comments does not look right but your last check was also weeks ago, thanks.

@kilo-code-bot

kilo-code-bot Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

To use Kilo from GitHub you first need to link your GitHub account to Kilo. Link your Kilo account to continue. After linking, mention me again in this issue or pull request.

@WebReflection

Copy link
Copy Markdown
Contributor

@kilocode-bot can you please check again? The opencode change without surrounding comments does not look right but your last check was also weeks ago, thanks.

@kilo-code-bot

kilo-code-bot Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Checked again at head 38ad122. Short answer: the logic is correct and safe, but WebReflection's instinct is partially right — one part of that hunk is under-documented.

On "no surrounding comments": the shared-file hunk in packages/opencode/src/provider/provider.ts is wrapped in a kilocode_change start/end block with a rationale, so it's not undocumented in general. However, that same hunk silently drops the old ?? s.catalog[...] fallback without saying why. That removal is correct (a catalog-only model would crash in resolveSDK / get no auth token), but it should be called out explicitly in the comment since it's an extra behavior change riding along with the "gate on credentials" fix.

Other non-blocking findings:

  1. hasKiloCredentials() hardcodes KILO_API_KEY instead of reading it from the provider's env array like the loader's hasKey does — small drift risk if that config ever adds a second env var.
  2. test/provider/provider.test.ts: the deleted upstream test (getSmallModel ignores model IDs without family metadata) should be restored verbatim rather than removed — it would still pass and keeps this shared file's diff-from-upstream at zero (better fork hygiene, avoids future merge conflicts).
  3. Minor: loosen-typed params (cfg, auth: unknown) could use existing Config.Info/Auth.Info types; stale comment in test/kilocode/sessions/ensure-title-mark.test.ts:221-223 still describes old behavior.

All production callers of getSmallModel already handle undefined by falling back to the session's own model, so no regression there.

Recommendation: Merge-able as is, but I'd suggest adding a one-line comment explaining the s.catalog removal and restoring the deleted upstream test before merging, since those are quick and remove the "doesn't look right" impression.

@WebReflection WebReflection left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary here, it's a matter of a comment that can be discoverable later on if needed, I think this looks good to me.

reference: #13192 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Session window does not change name locally

2 participants